Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ICU-22541 Fix MacOS 14 default timezone issue #2669

Merged
merged 1 commit into from
Oct 13, 2023
Merged

ICU-22541 Fix MacOS 14 default timezone issue #2669

merged 1 commit into from
Oct 13, 2023
+18 −6

Conversation

FrankYFTang
Copy link
Contributor

@FrankYFTang FrankYFTang commented Oct 11, 2023 •
edited by markusicu
Loading

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22541
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@FrankYFTang
Copy link
Contributor Author

@Robbos @syg

markusicu
markusicu previously approved these changes Oct 11, 2023
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@richgillam @pedberg-icu et al. ok?

icu4c/source/common/putil.cpp Outdated Show resolved Hide resolved
@srl295
Copy link
Member

srl295 commented Oct 11, 2023

Sigh, if only tzdb would have agreed (I asked) to include an actual ID in the file. Instead, we deal with randomness such as ../posixrules

srl295
srl295 previously approved these changes Oct 11, 2023
View reviewed changes
@syg
Copy link

syg commented Oct 11, 2023 •
edited
Loading

This might not be the right place to discuss: does this fix the Ubuntu 18.04 breakage we saw with Chrome, which was due to /etc/localtime -> /usr/share/zoneinfo/America/New_York -> /usr/share/zoneinfo/posixrules? (To compare, my Linux box (Google's corp Linux distro) has /usr/share/zoneinfo/posixrules -> /usr/share/zoneinfo/America/New_York instead.)

Edit: Rephrased as question.

@srl295
Copy link
Member

srl295 commented Oct 11, 2023

This might not be the right place to discuss: this won't fix the Ubuntu 18.04 breakage we saw with Chrome,

it seems like it is the right place to discuss since the ticket claims to be about that specific breakage

@markusicu
Copy link
Member

does this fix the Ubuntu 18.04 breakage we saw with Chrome, which was due to /etc/localtime -> /usr/share/zoneinfo/America/New_York -> /usr/share/zoneinfo/posixrules?

I think not, because we should see tzZoneInfoTailPtr != nullptr.
We should be able to fix that by changing the check to

if (tzZoneInfoTailPtr == nullptr ||
        uprv_strcmp(tzZoneInfoTailPtr + tzZoneInfoTailLen, "posixrules") == 0) {

richgillam
richgillam previously approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks okay, but I'm interested in what Debbie Goldsmith has to say in the email discussion that accompanied this PR. From talking to Peter, it sounds possible that we're doing something weird in the way we write the time zone files to the disk, and that therefore the issue might not be with ICU so much as with our own time-zone-updating code.

This is all stuff I personally know diddly-squat about, but with Peter leaving, I guess I'll have to learn. But I think this is probably okay, at least for now.

@FrankYFTang FrankYFTang dismissed stale reviews from richgillam, srl295, and markusicu via 119a18a October 12, 2023 16:28
FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Oct 12, 2023
@FrankYFTang
ICU-22541 Fix MacOS 14 default timezone issue
7d58692 
See unicode-org#2669
@FrankYFTang FrankYFTang force-pushed the ICU-22541-fixTZreadlink branch from 119a18a to 7d58692 Compare October 12, 2023 16:30
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

@markusicu markusicu self-assigned this Oct 12, 2023
@markusicu
Copy link
Member

markusicu commented Oct 12, 2023 •
edited
Loading

Test failures. Please change icu4c/source/test/depstest/dependencies.txt:

  • line 25: remove realpath_function
  • lines 113..115: remove the realpath_function group
  • line 117: add readlink realpath after readdir
  • line 871: remove realpath_function

@FrankYFTang FrankYFTang force-pushed the ICU-22541-fixTZreadlink branch from 7d58692 to dad7f7c Compare October 12, 2023 21:51
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/common/putil.cpp is different
  • icu4c/source/test/depstest/dependencies.txt is now changed in the branch

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@richgillam
Copy link
Contributor

FWIW, I added commentary to the JIRA ticket explaining the underlying cause and recommended solution for this issue.

richgillam
richgillam previously approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@richgillam richgillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't speak to Markus's comments, but Frank's logic here looks correct to me. Based on my investigation of the code (see my comments in the JIRA ticket), I think that algorithm will work perfectly well as a long-term solution.

markusicu
markusicu requested changes Oct 12, 2023
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please un-revert the "posixrules" change that you accepted yesterday and reverted today.

@FrankYFTang
Copy link
Contributor Author

opps.. I commit it on github, and then forget to pull into my local branch, and then I added the dependecy stuff and force push, causing the revert. Sorry.

@FrankYFTang FrankYFTang force-pushed the ICU-22541-fixTZreadlink branch from eb7ec5e to 87c3133 Compare October 12, 2023 23:34
@jira-pull-request-webhook
Copy link

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

markusicu
markusicu approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@markusicu markusicu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@FrankYFTang FrankYFTang merged commit ca631ea into unicode-org:maint/maint-74 Oct 13, 2023
96 checks passed
@FrankYFTang FrankYFTang deleted the ICU-22541-fixTZreadlink branch October 13, 2023 17:38
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 15, 2023
@makotokato
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba
66ef209 
Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 16, 2023
@marco-c
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba
f77f5ec 
Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013

UltraBlame original commit: 91657cb1296c904004ace38877b9ca91f502be3f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 16, 2023
@marco-c
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba
947d9e6 
Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013

UltraBlame original commit: 91657cb1296c904004ace38877b9ca91f502be3f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 16, 2023
@marco-c
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba
9d9f92c 
Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013

UltraBlame original commit: 91657cb1296c904004ace38877b9ca91f502be3f
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 16, 2023
@makotokato
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba
4dc36f3 
Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 16, 2023
@makotokato
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba, a…
a24cb95 
…=dmeehan

Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 16, 2023
@makotokato
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba, a…
20523fd 
…=dmeehan

Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013
github-actions bot pushed a commit to Floorp-Projects/Floorp that referenced this pull request Oct 22, 2023
@makotokato
Bug 1856428 - MacOS Sonoma doesn't return correct timezone. r=anba, a…
436de07 
…=dmeehan

Import from https://unicode-org.atlassian.net/browse/ICU-22541
(unicode-org/icu#2669).

Differential Revision: https://phabricator.services.mozilla.com/D191013
@DavidDudson DavidDudson mentioned this pull request Oct 22, 2023
Closed
MrJithil pushed a commit to MrJithil/icu that referenced this pull request Oct 25, 2023
@FrankYFTang @MrJithil
ICU-22541 Fix MacOS 14 default timezone issue
49dde0d 
See unicode-org#2669
MrJithil pushed a commit to MrJithil/icu that referenced this pull request Oct 25, 2023
@FrankYFTang @MrJithil
ICU-22541 Fix MacOS 14 default timezone issue
81e26e0 
See unicode-org#2669
@MrJithil MrJithil mentioned this pull request Oct 25, 2023
Draft
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusicu markusicu markusicu approved these changes

@richgillam richgillam richgillam left review comments

@yumaoka yumaoka Awaiting requested review from yumaoka

@garywade garywade Awaiting requested review from garywade

@pedberg-icu pedberg-icu Awaiting requested review from pedberg-icu

@srl295 srl295 Awaiting requested review from srl295

Assignees

@markusicu markusicu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@FrankYFTang @srl295 @syg @markusicu @richgillam